module: improve error message for top-level await in CommonJS #55874

mertcanaltin · 2024-11-15T20:44:24Z

Added a specific error message for using top-level await in CommonJS modules. The error now suggests using ESM or wrapping await in an async function for clarity.

Fixes: #55776

codecov · 2024-11-15T22:05:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.53%. Comparing base (4ee87b8) to head (01b4b6d).
Report is 72 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55874      +/-   ##
==========================================
+ Coverage   88.00%   88.53%   +0.53%     
==========================================
  Files         656      657       +1     
  Lines      189000   189866     +866     
  Branches    35995    36453     +458     
==========================================
+ Hits       166320   168092    +1772     
+ Misses      15840    14981     -859     
+ Partials     6840     6793      -47

Files with missing lines	Coverage Δ
src/node_contextify.cc	`81.66% <100.00%> (+0.29%)`	⬆️

... and 125 files with indirect coverage changes

GeoffreyBooth

Thank you for tackling this! I’d love to see this issue addressed.

src/node_contextify.cc

GeoffreyBooth · 2024-11-16T01:12:06Z

src/node_contextify.cc

+  for (const auto& error_message : throws_only_in_cjs_error_messages) {
+    if (message_view.find(error_message) != std::string_view::npos) {
+      isolate->ThrowException(v8::Exception::SyntaxError(
+          String::NewFromUtf8(
+              isolate,
+              "Top-level await is not supported in CommonJS. "
+              "Consider using ESM or wrap await in an async function.")
+              .ToLocalChecked()));


It seems like this message is being printed for any of the “throws only in CJS” error messages? I would think that we would want to print this only when the user wrote a top-level await in a file that didn’t parse successfully as ESM.

Also in general our messages don’t use “ESM” as that’s not a widely known term. I think we usually write “module syntax” or “import/export syntax”.

thank you very much for the improvement suggestions, I wonder if you could comment on the broken test/es-module/test-esm-detect-ambiguous.mjs tests, I am trying to improve them

in addition, I wonder if it would be better to include my newly added test/parallel/test-commonjs-top-level-await.js test in test-esm-detect-ambiguous.mjs

What test failure do you see?

in addition, I wonder if it would be better to include my newly added test/parallel/test-commonjs-top-level-await.js test in test-esm-detect-ambiguous.mjs

Yes, my preference would be to keep all the syntax detection tests together. If you could follow the pattern of the other related tests that would be great.

I saw this
https://github.com/nodejs/node/actions/runs/11863231944/job/33064268346?pr=55874

I saw this
nodejs/node/actions/runs/11863231944/job/33064268346?pr=55874

I don't see the failure in this output. When you run the tests locally, what fails?

when I run the tests locally:
./node test/es-module/test-esm-detect-ambiguous.mjs

I see that 7 tests are exploding, I think the code we do error handling is crushing the expectations here

failing tests:

✖ does not warn when there are no package.json
✖ permits declaration of CommonJS module variables
✖ throws on undefined require when top-level await triggers ESM parsing
✖ still throws on await in an ordinary sync function
✖ permits top-level await above import/export syntax
✖ reports unfinished top-level await
✖ permits top-level await

I'm trying to fix this place.

@GeoffreyBooth would you have any suggestions for these test failures?

I think those failures are probably legit and the implementation needs to be adjusted so that they pass.

I think those failures are probably legit and the implementation needs to be adjusted so that they pass.

thank you, as you said I will try to fix the tests as legitimate

I made an update for this place, many thanks for the suggestions @GeoffreyBooth

GeoffreyBooth

Thanks for taking another pass at this. I mention this in the particular notes, but in general:

Please don't change lines based on personal preference or readability. We value the ability for git blame to point directly to the commit/PR that added or meaningfully changed a particular line, and that ability is diluted when unrelated PRs rewrite lines of code.
Very few, if any, tests should change as a result of your PR: only tests specifically related to top-level await in CommonJS. Any other tests that fail as a result of your changes mean that there's a bug in the implementation that needs to be addressed. My guess is that the bug is that the new code isn't checking that the error message being thrown is specifically about top-level await. If there aren't currently tests for top-level await in CommonJS and your new error message, one or more should be added.

This is close; another pass and I think you may get it there. Thanks for your effort!

src/node_contextify.cc

test/es-module/test-esm-detect-ambiguous.mjs

Co-authored-by: Geoffrey Booth <[email protected]>

mertcanaltin · 2024-11-30T20:40:18Z

Thanks for taking another pass at this. I mention this in the particular notes, but in general:

Please don't change lines based on personal preference or readability. We value the ability for git blame to point directly to the commit/PR that added or meaningfully changed a particular line, and that ability is diluted when unrelated PRs rewrite lines of code.

Very few, if any, tests should change as a result of your PR: only tests specifically related to top-level await in CommonJS. Any other tests that fail as a result of your changes mean that there's a bug in the implementation that needs to be addressed. My guess is that the bug is that the new code isn't checking that the error message being thrown is specifically about top-level await. If there aren't currently tests for top-level await in CommonJS and your new error message, one or more should be added.

This is close; another pass and I think you may get it there. Thanks for your effort!

thank you very much for the review, I will be sending a new commit following what you said

mertcanaltin · 2024-12-02T18:28:41Z

Greetings, when I did as you suggested to cover only the top level error, I saw that all tests passed except 1 test

GeoffreyBooth

Thanks, this is much better. What’s the one test that is failing?

test/es-module/test-esm-detect-ambiguous.mjs

GeoffreyBooth · 2024-12-02T23:03:51Z

test/es-module/test-esm-detect-ambiguous.mjs

+
+    match(
+      stderr,
+      /await is only valid in async functions and the top level bodies of modules/


The assertion should include some of the new text you’re adding, such as “To use top-level await,” to verify that your new error message is what’s getting printed.

Co-authored-by: Geoffrey Booth <[email protected]>

mertcanaltin · 2024-12-03T15:00:24Z

Thanks, this is much better. What’s the one test that is failing?

I saw wrong, all the tests passed my local. 🙏

GeoffreyBooth · 2024-12-03T21:07:41Z

I saw wrong, all the tests passed my local. 🙏

There’s a test failing in here, in the relevant file: https://github.com/nodejs/node/actions/runs/12133213207/job/33828380981?pr=55874

@cjihrig why isn’t the CI output showing me which test is failing within that file?

cjihrig · 2024-12-03T21:21:36Z

why isn’t the CI output showing me which test is failing within that file?

No clue. But the output is incomplete. There is no summary at the end either. It looks like maybe the Python runner killed the Node process or otherwise truncated its output.

cjihrig · 2024-12-03T21:39:50Z

@GeoffreyBooth just FYI - IIRC, you asked me a similar question in Slack last year. There was output truncated only in GitHub Actions. Then you asked me to find a test run where truncation was happening without the use of node:test, and I did. I believe @MoLow was a part of that thread as well.

GeoffreyBooth · 2024-12-04T01:24:08Z

IIRC, you asked me a similar question in Slack last year.

Yes, last August: https://openjs-foundation.slack.com/archives/C019Y2T6STH/p1691860008875109. I’m lucky if I can remember last month, much less last year. Sorry to repeat. I also opened #49120 but that got closed.

mertcanaltin · 2024-12-04T09:17:45Z

I saw wrong, all the tests passed my local. 🙏

There’s a test failing in here, in the relevant file: https://github.com/nodejs/node/actions/runs/12133213207/job/33828380981?pr=55874

@cjihrig why isn’t the CI output showing me which test is failing within that file?

yes, ı view my local enviorement this test is fail

test at test/es-module/test-esm-detect-ambiguous.mjs:367:5
✖ does not warn when there are no package.json (63.625667ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected
  
  + "(node:18411) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/mert/Desktop/openSource/node/test/fixtures/es-modules/loose.js is not specified and it doesn't parse as CommonJS.\n" +
  +   'Reparsing as ES module because module syntax was detected. This incurs a performance overhead.\n' +
  +   'To eliminate this warning, add "type": "module" to /Users/mert/package.json.\n' +
  +   '(Use `node --trace-warnings ...` to show where the warning was created)\n'
  - ''
  
      at TestContext.<anonymous> (file:///Users/mert/Desktop/openSource/node/test/es-module/test-esm-detect-ambiguous.mjs:372:7)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Test.run (node:internal/test_runner/test:932:9)
      at async Promise.all (index 3)
      at async Suite.run (node:internal/test_runner/test:1310:7)
      at async Promise.all (index 6)
      at async Suite.run (node:internal/test_runner/test:1310:7)
      at async startSubtestAfterBootstrap (node:internal/test_runner/harness:297:3) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: '(node:18411) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/mert/Desktop/openSource/node/test/fixtures/es-modules/loose.js is not specified and it doesn\'t parse as CommonJS.\nReparsing as ES module because module syntax was detected. This incurs a performance overhead.\nTo eliminate this warning, add "type": "module" to /Users/mert/package.json.\n(Use `node --trace-warnings ...` to show where the warning was created)\n',
    expected: '',
    operator: 'strictEqual'
  }

mertcanaltin · 2024-12-10T20:13:53Z

greetings I tried to make the error message more descriptive, in addition I tried to throw it when there is a file that cannot be successfully parsed as ESM

mertcanaltin requested review from GeoffreyBooth and joyeecheung November 15, 2024 20:44

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Nov 15, 2024

GeoffreyBooth reviewed Nov 16, 2024

View reviewed changes

mertcanaltin added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Nov 27, 2024

mertcanaltin force-pushed the dev-55776 branch 2 times, most recently from 3dccce9 to 76f87e4 Compare November 29, 2024 21:18

module: improve error handling for top-level await in CommonJS

c9531ef

mertcanaltin force-pushed the dev-55776 branch from 76f87e4 to c9531ef Compare November 29, 2024 21:21

repair

9dfd63d

mertcanaltin added review wanted PRs that need reviews. and removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Nov 30, 2024

mertcanaltin requested a review from GeoffreyBooth November 30, 2024 16:36

GeoffreyBooth reviewed Nov 30, 2024

View reviewed changes

mertcanaltin and others added 11 commits November 30, 2024 23:33

Update src/node_contextify.cc

1b637d6

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

04e570f

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

1e12966

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

11d788e

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

2f8e7d4

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

bd58f77

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

2e77c7c

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

42387e9

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

34e2c3c

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

5040118

Co-authored-by: Geoffrey Booth <[email protected]>

Update test/es-module/test-esm-detect-ambiguous.mjs

8e720b1

Co-authored-by: Geoffrey Booth <[email protected]>

improve error handling for top-level await in CommonJS

ef625f3

Mert Can Altin added 2 commits December 2, 2024 21:09

test repair

bf87e8d

test repair

f9d84b2

added test for top-level

b1f54e3

mertcanaltin requested a review from GeoffreyBooth December 2, 2024 19:31

GeoffreyBooth reviewed Dec 2, 2024

View reviewed changes

GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Dec 2, 2024

Update test-esm-detect-ambiguous.mjs

94f5a35

Co-authored-by: Geoffrey Booth <[email protected]>

mert added 5 commits December 10, 2024 22:10

fix: Improve top-level await handling and test reliability

9099572

lint

b76fdb4

test repair

c3a13fb

test repair

bb74388

Refine error handling for top-level await in CommonJS modules

01b4b6d

mertcanaltin requested a review from GeoffreyBooth December 10, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: improve error message for top-level await in CommonJS #55874

module: improve error message for top-level await in CommonJS #55874

mertcanaltin commented Nov 15, 2024

codecov bot commented Nov 15, 2024 •

edited

Loading

GeoffreyBooth left a comment

GeoffreyBooth Nov 16, 2024

mertcanaltin Nov 16, 2024

GeoffreyBooth Nov 16, 2024

mertcanaltin Nov 16, 2024

GeoffreyBooth Nov 18, 2024

mertcanaltin Nov 20, 2024

mertcanaltin Nov 26, 2024

GeoffreyBooth Nov 27, 2024

mertcanaltin Nov 27, 2024

mertcanaltin Nov 29, 2024

GeoffreyBooth left a comment

mertcanaltin commented Nov 30, 2024

mertcanaltin commented Dec 2, 2024

GeoffreyBooth left a comment

GeoffreyBooth Dec 2, 2024

mertcanaltin commented Dec 3, 2024

GeoffreyBooth commented Dec 3, 2024

cjihrig commented Dec 3, 2024

cjihrig commented Dec 3, 2024

GeoffreyBooth commented Dec 4, 2024

mertcanaltin commented Dec 4, 2024

mertcanaltin commented Dec 10, 2024

module: improve error message for top-level await in CommonJS #55874

Are you sure you want to change the base?

module: improve error message for top-level await in CommonJS #55874

Conversation

mertcanaltin commented Nov 15, 2024

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth left a comment

Choose a reason for hiding this comment

mertcanaltin commented Nov 30, 2024

mertcanaltin commented Dec 2, 2024

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertcanaltin commented Dec 3, 2024

GeoffreyBooth commented Dec 3, 2024

cjihrig commented Dec 3, 2024

cjihrig commented Dec 3, 2024

GeoffreyBooth commented Dec 4, 2024

mertcanaltin commented Dec 4, 2024

mertcanaltin commented Dec 10, 2024

codecov bot commented Nov 15, 2024 •

edited

Loading